refactor: clean up transaction model validations#2895
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors transaction validation across the xrpl.js library by consolidating scattered validation logic into reusable helper functions ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (5)
packages/xrpl/test/models/NFTokenCancelOffer.test.ts (2)
1-21:⚠️ Potential issueAdd missing imports for test dependencies.
The test is using
assertandvalidatebut they are not imported. Add the following imports at the beginning of the file:+import { assert } from 'chai' +import { validate } from '../validate'🧰 Tools
🪛 ESLint
[error] 1-48: This module could be parsed as a valid script.
(import/unambiguous)
🪛 GitHub Actions: Node.js CI
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
23-33:⚠️ Potential issueAdd import for assertInvalid helper.
The test is using the new
assertInvalidhelper but it's not imported. Add the following import:+import { assertInvalid } from '../testUtils'The error message format aligns with the PR's objective of standardizing validation messages.
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 32-32: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/clawback.test.ts (1)
81-81:⚠️ Potential issueFix async function call in MPT Clawback test.
Similar to the previous issue, this test case also needs to handle the async validation correctly.
Apply this diff to fix the async issues:
- assert.doesNotThrow(() => validate(validClawback)) + await assert.doesNotReject(async () => validate(validClawback))Also update the test function to be async:
- it(`verifies valid MPT Clawback`, function () { + it(`verifies valid MPT Clawback`, async function () {🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 81-81: Functions that return promises must be async
[error] 81-81: Unsafe return of an
anytyped valuepackages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts (1)
1-2:⚠️ Potential issueAdd missing imports to fix pipeline failures.
The file is using undefined functions that need to be imported.
Add the following imports at the beginning of the file:
+import { validate } from '../../src/models' +import { validateMPTokenIssuanceDestroy } from '../../src/models/transactions/MPTokenIssuanceDestroy' +import { assertTxIsValid, assertTxValidationError } from '../testUtils' const TOKEN_ID = '000004C463C52827307480341125DA0577DEFC38405B0E3E'Also, define the utility functions:
+const assertValid = (tx: any): void => + assertTxIsValid(tx, validateMPTokenIssuanceDestroy) +const assertInvalid = (tx: any, message: string): void => + assertTxValidationError(tx, validateMPTokenIssuanceDestroy, message)🧰 Tools
🪛 ESLint
[error] 1-31: This module could be parsed as a valid script.
(import/unambiguous)
packages/xrpl/test/models/NFTokenBurn.test.ts (1)
1-3:⚠️ Potential issueAdd missing imports to fix pipeline failures.
The file is using undefined functions that need to be imported.
Add the following imports and utility functions:
+import { validate } from '../../src/models' +import { validateNFTokenBurn } from '../../src/models/transactions/NFTokenBurn' +import { assertTxIsValid, assertTxValidationError } from '../testUtils' const TOKEN_ID = '00090032B5F762798A53D543A014CAF8B297CFF8F2F937E844B17C9E00000003' +const assertValid = (tx: any): void => + assertTxIsValid(tx, validateNFTokenBurn) +const assertInvalid = (tx: any, message: string): void => + assertTxValidationError(tx, validateNFTokenBurn, message)🧰 Tools
🪛 ESLint
[error] 1-35: This module could be parsed as a valid script.
(import/unambiguous)
🧹 Nitpick comments (37)
packages/xrpl/test/models/AMMWithdraw.test.ts (1)
88-152: Consider using undefined assignment instead of delete operator.While the validation refactoring looks good, the use of
deleteoperator in test cases could impact performance.Apply this diff to improve performance:
- delete withdraw.Asset + withdraw.Asset = undefined- delete withdraw.Asset2 + withdraw.Asset2 = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 101-101: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts (2)
14-14: Consider using a more specific type instead ofany.Using
anytype bypasses TypeScript's type checking. Consider defining an interface for the AMMVote transaction type to improve type safety.interface AMMVoteTransaction { TransactionType: 'AMMVote' Account: string Asset: { currency: string } Asset2: { currency: string issuer: string } TradingFee: number Sequence: number }
37-37: Consider using undefined assignment instead of delete operator.For better performance, consider replacing
deleteoperations with undefined assignments:-delete vote.Asset +vote.Asset = undefined -delete vote.Asset2 +vote.Asset2 = undefined -delete vote.TradingFee +vote.TradingFee = undefinedAlso applies to: 49-49, 61-61
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts (1)
29-29: Consider avoiding the delete operator.Using the
deleteoperator can impact performance. Consider using an undefined assignment instead.- delete tx.OracleDocumentID + tx.OracleDocumentID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts (3)
4-7: Consider using proper types instead ofany.The helper functions could benefit from proper type definitions to maintain type safety. Consider using the transaction type from the validation function.
-const assertValid = (tx: any): void => +const assertValid = (tx: XChainAccountCreateCommit): void => assertTxIsValid(tx, validateXChainAccountCreateCommit) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<XChainAccountCreateCommit>, message: string): void => assertTxValidationError(tx, validateXChainAccountCreateCommit, message)
15-15: Consider using proper type instead ofany.For better type safety and IDE support, consider using the proper transaction type.
-let tx: any +let tx: XChainAccountCreateCommit
45-45: Consider alternatives to thedeleteoperator.The
deleteoperator can impact performance. Consider using undefined assignment instead.-delete tx.XChainBridge +tx.XChainBridge = undefined -delete tx.SignatureReward +tx.SignatureReward = undefined -delete tx.Destination +tx.Destination = undefined -delete tx.Amount +tx.Amount = undefinedAlso applies to: 57-57, 75-75, 87-87
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts (3)
7-9: Consider using more specific types instead ofany.While
anyworks, using more specific types would improve type safety and maintainability. Consider creating an interface for the transaction type.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateAMMDeposit) -const assertInvalid = (tx: any, message: string): void => +interface AMMDepositTransaction { + TransactionType: 'AMMDeposit' + Account: string + Asset: any // Replace with specific type + Asset2: any // Replace with specific type + [key: string]: any +} +const assertValid = (tx: AMMDepositTransaction): void => assertTxIsValid(tx, validateAMMDeposit) +const assertInvalid = (tx: Partial<AMMDepositTransaction>, message: string): void =>
22-22: Consider using the specific transaction type instead ofany.Replace
anywith the suggestedAMMDepositTransactiontype to improve type safety.-let deposit: any +let deposit: AMMDepositTransaction
77-93: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. Consider using undefined assignment instead.-delete deposit.Asset +deposit.Asset = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts (2)
14-14: Consider using proper transaction type instead ofany.Replace
anywith the proper transaction type to improve type safety and catch potential issues at compile time.- let escrow: any + let escrow: EscrowCreate
36-36: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. Consider using undefined assignment instead:- delete escrow.Amount + escrow.Amount = undefined - delete escrow.Destination + escrow.Destination = undefined - delete escrow.CancelAfter - delete escrow.FinishAfter + escrow.CancelAfter = undefined + escrow.FinishAfter = undefined - delete escrow.Condition - delete escrow.FinishAfter + escrow.Condition = undefined + escrow.FinishAfter = undefinedAlso applies to: 42-42, 84-85, 94-95
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelClaim.test.ts (2)
4-7: Consider using a more specific type instead ofany.While
anyworks, using a more specific type would improve type safety. Consider creating an interface or type that represents the expected transaction structure.-const assertValid = (tx: any): void => +interface PaymentChannelClaimTx { + Account?: string + TransactionType: 'PaymentChannelClaim' + Channel?: string + Balance?: string + Amount?: string + Signature?: string + PublicKey?: string +} +const assertValid = (tx: PaymentChannelClaimTx): void => assertTxIsValid(tx, validatePaymentChannelClaim) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: PaymentChannelClaimTx, message: string): void => assertTxValidationError(tx, validatePaymentChannelClaim, message)
15-15: Use the specific transaction type for better type safety.Replace
anywith the specific transaction type to improve type safety and code maintainability.- let channel: any + let channel: PaymentChannelClaimTxpackages/xrpl/test/models/offerCancel.test.ts (1)
47-48: Consider alternative todeleteoperator.While the error message update follows the new consistent format, static analysis flags the use of the
deleteoperator due to potential performance impact.Although the performance impact is negligible in test context, you could consider using undefined assignment instead:
-delete offer.OfferSequence +offer.OfferSequence = undefinedHowever, using
deletemore clearly expresses the intent to test a missing field. The choice is yours based on whether you prioritize performance or clarity in tests.🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelCreate.test.ts (2)
4-7: Consider using a more specific type for thetxparameter.Instead of using
any, consider creating an interface or type that represents the expected shape of a PaymentChannelCreate transaction. This would provide better type safety and documentation.-const assertValid = (tx: any): void => +interface PaymentChannelCreateTransaction { + Account: string + TransactionType: 'PaymentChannelCreate' + Amount: string + Destination: string + SettleDelay: number + PublicKey: string + CancelAfter?: number + DestinationTag?: number + SourceTag?: number +} +const assertValid = (tx: Partial<PaymentChannelCreateTransaction>): void => assertTxIsValid(tx, validatePaymentChannelCreate) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<PaymentChannelCreateTransaction>, message: string): void => assertTxValidationError(tx, validatePaymentChannelCreate, message)
15-15: Use the specific transaction type instead ofany.Replace
anywith the suggestedPaymentChannelCreateTransactiontype for better type safety.- let channel: any + let channel: PaymentChannelCreateTransactionpackages/xrpl/test/models/DIDSet.test.ts (2)
4-6: Consider using a more specific type instead ofany.While
anyworks, using a more specific type would improve type safety. Consider creating an interface for the transaction type or using the existing transaction type from the codebase.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateDIDSet) -const assertInvalid = (tx: any, message: string): void => +interface DIDSetTransaction { + Account: string + Data?: string + DIDDocument?: string + URI?: string + Fee: string + Flags: number + Sequence: number + TransactionType: 'DIDSet' +} +const assertValid = (tx: DIDSetTransaction): void => assertTxIsValid(tx, validateDIDSet) +const assertInvalid = (tx: DIDSetTransaction, message: string): void =>
51-60: Consider using undefined assignment instead of delete operator.While the current implementation works, the delete operator can have performance implications. Although this is test code where performance is less critical, for consistency with best practices, consider using undefined assignments.
- delete tx.Data - delete tx.DIDDocument - delete tx.URI + tx.Data = undefined + tx.DIDDocument = undefined + tx.URI = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 54-54: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/trustSet.test.ts (2)
14-14: Consider using a more specific type instead ofany.Using
anytype bypasses TypeScript's type checking. Consider creating an interface or type for the TrustSet transaction to improve type safety.Example:
interface TrustSetTransaction { TransactionType: 'TrustSet' Account: string LimitAmount: { currency: string issuer: string value: string } QualityIn?: number QualityOut?: number Flags?: { tfSetFreeze?: boolean tfSetDeepFreeze?: boolean } }
40-41: Consider using undefined assignment instead of delete operator.Using the
deleteoperator can impact performance. Consider using an undefined assignment instead.-delete trustSet.LimitAmount +trustSet.LimitAmount = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/NFTokenModify.test.ts (1)
11-61: Consider adding test for URI length validation.While the test coverage is good for basic URI validation, consider adding a test case for maximum URI length validation, which is a common requirement for NFT metadata.
Here's a suggested test case:
it(`throws w/ URI exceeding maximum length`, function () { const invalid = { TransactionType: 'NFTokenModify', Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm', NFTokenID: TOKEN_ID, Fee: '5000000', Sequence: 2470665, URI: stringToHex('x'.repeat(257)), // Assuming 256 bytes is the maximum } as any assertInvalid(invalid, 'NFTokenModify: URI length cannot exceed 256 bytes') })🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/baseTransaction.test.ts (1)
11-248: Consider adding more test cases for edge cases.The test coverage is comprehensive, but consider adding test cases for:
- Multiple invalid fields in a single transaction
- Boundary values for numeric fields
- Empty strings vs null/undefined values
Would you like me to help generate these additional test cases?
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
31-31: Replace delete operator with undefined assignment.The
deleteoperator can impact performance. Consider using an undefined assignment instead.-delete tx.DomainID +tx.DomainID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDelete.test.ts (1)
37-38: Consider using undefined assignment instead of delete operator.For better performance, consider replacing
deleteoperations with undefined assignments:- delete ammDelete.Asset + ammDelete.Asset = undefined - delete ammDelete.Asset2 + ammDelete.Asset2 = undefinedAlso applies to: 49-50
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts (1)
29-30: Consider using undefined assignment instead of delete operator.For better performance, consider replacing the
deleteoperation with an undefined assignment:- delete ticketCreate.TicketCount + ticketCreate.TicketCount = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelFund.test.ts (1)
33-34: Consider using undefined assignments instead of delete operators.For better performance, consider replacing multiple
deleteoperations with undefined assignments:- delete channel.Expiration + channel.Expiration = undefined - delete channel.Amount + channel.Amount = undefined - delete channel.Channel + channel.Channel = undefinedAlso applies to: 39-40, 45-46
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts (1)
44-45: Consider using undefined assignment instead of delete operator.For better performance, consider replacing the
deleteoperation with an undefined assignment:- delete tx.XChainBridge + tx.XChainBridge = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateBridge.test.ts (1)
44-44: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. While this is less critical in test code, consider using undefined assignment for consistency with static analysis recommendations.- delete tx.XChainBridge + tx.XChainBridge = undefined - delete tx.SignatureReward + tx.SignatureReward = undefinedAlso applies to: 56-56
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCommit.test.ts (1)
43-43: Consider using undefined assignment instead of delete operator.Similar to other test files, replace
deletewith undefined assignments for better performance.- delete tx.XChainBridge + tx.XChainBridge = undefined - delete tx.XChainClaimID + tx.XChainClaimID = undefined - delete tx.Amount + tx.Amount = undefinedAlso applies to: 55-55, 73-73
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMCreate.test.ts (1)
36-36: Consider using undefined assignment instead of delete operator.Replace
deletewith undefined assignments for consistency with static analysis recommendations.- delete ammCreate.Amount + ammCreate.Amount = undefined - delete ammCreate.Amount2 + ammCreate.Amount2 = undefined - delete ammCreate.TradingFee + ammCreate.TradingFee = undefinedAlso applies to: 48-48, 60-60
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainClaim.test.ts (1)
44-44: Consider using undefined assignment instead of delete operator.Replace
deletewith undefined assignments for better performance and consistency.- delete tx.XChainBridge + tx.XChainBridge = undefined - delete tx.XChainClaimID + tx.XChainClaimID = undefined - delete tx.Destination + tx.Destination = undefined - delete tx.Amount + tx.Amount = undefinedAlso applies to: 56-56, 68-68, 86-86
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMClawback.test.ts (1)
45-45: Consider using undefined assignment for better performance.Multiple instances of the
deleteoperator are used to remove properties. While acceptable in tests, consider using undefined assignment for better performance:-delete ammClawback.Amount +ammClawback.Amount = undefinedAlso applies to: 55-55, 73-73, 91-91
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts (1)
68-68: Consider using undefined assignment for better performance.Similar to other test files, replace delete operator with undefined assignment:
-delete bid.Asset +bid.Asset = undefinedAlso applies to: 80-80
🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts (1)
4-7: LGTM! Consider performance improvements.The introduction of helper functions and explicit typing improves code maintainability and type safety.
For better performance, consider replacing
deleteoperations withundefinedassignments in test cases:-delete tx.Amount +tx.Amount = undefinedThis change would address the static analysis warnings about the
deleteoperator's performance impact.Also applies to: 15-15
packages/xrpl/test/testUtils.ts (2)
60-69: Consider improving type safety and documentation.The function implementation looks good, but consider these improvements:
- Replace
anywith a more specific type for thetxparameter- Add example usage in the JSDoc comment
/** * Check that a transaction error validation fails properly. * + * @example + * ```typescript + * const tx = { TransactionType: 'Payment', ... } + * assertTxIsValid(tx, validatePayment) + * ``` * * @param tx The transaction that should fail validation. * @param validateTx The transaction-specific validation function (e.g. `validatePayment`). */ -export function assertTxIsValid(tx: any, validateTx: (tx: any) => void): void { +export function assertTxIsValid( + tx: Record<string, unknown>, + validateTx: (tx: Record<string, unknown>) => void +): void { assert.doesNotThrow(() => validateTx(tx)) assert.doesNotThrow(() => validate(tx)) }
71-85: Consider improving type safety and documentation.The function implementation looks good, but consider these improvements:
- Replace
anywith a more specific type for thetxparameter- Add example usage in the JSDoc comment
/** * Check that a transaction error validation fails properly. * + * @example + * ```typescript + * const tx = { TransactionType: 'Payment' } // missing required field + * assertTxValidationError(tx, validatePayment, 'invalid field') + * ``` * * @param tx The transaction that should fail validation. * @param validateTx The transaction-specific validation function (e.g. `validatePayment`). * @param errorMessage The error message that should be included in the error. */ export function assertTxValidationError( - tx: any, - validateTx: (tx: any) => void, + tx: Record<string, unknown>, + validateTx: (tx: Record<string, unknown>) => void, errorMessage: string, ): void { assert.throws(() => validateTx(tx), ValidationError, errorMessage) assert.throws(() => validate(tx), ValidationError, errorMessage) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
.vscode/settings.json(1 hunks)packages/xrpl/test/models/AMMBid.test.ts(5 hunks)packages/xrpl/test/models/AMMClawback.test.ts(2 hunks)packages/xrpl/test/models/AMMCreate.test.ts(2 hunks)packages/xrpl/test/models/AMMDelete.test.ts(2 hunks)packages/xrpl/test/models/AMMDeposit.test.ts(5 hunks)packages/xrpl/test/models/AMMVote.test.ts(2 hunks)packages/xrpl/test/models/AMMWithdraw.test.ts(5 hunks)packages/xrpl/test/models/CredentialAccept.test.ts(2 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(2 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(2 hunks)packages/xrpl/test/models/DIDDelete.test.ts(2 hunks)packages/xrpl/test/models/DIDSet.test.ts(2 hunks)packages/xrpl/test/models/MPTokenAuthorize.test.ts(6 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(9 hunks)packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts(1 hunks)packages/xrpl/test/models/MPTokenIssuanceSet.test.ts(3 hunks)packages/xrpl/test/models/NFTokenAcceptOffer.test.ts(6 hunks)packages/xrpl/test/models/NFTokenBurn.test.ts(1 hunks)packages/xrpl/test/models/NFTokenCancelOffer.test.ts(2 hunks)packages/xrpl/test/models/NFTokenCreateOffer.test.ts(9 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(6 hunks)packages/xrpl/test/models/NFTokenModify.test.ts(5 hunks)packages/xrpl/test/models/XChainAccountCreateCommit.test.ts(2 hunks)packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainAddClaimAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainClaim.test.ts(2 hunks)packages/xrpl/test/models/XChainCommit.test.ts(2 hunks)packages/xrpl/test/models/XChainCreateBridge.test.ts(2 hunks)packages/xrpl/test/models/XChainCreateClaimID.test.ts(2 hunks)packages/xrpl/test/models/XChainModifyBridge.test.ts(2 hunks)packages/xrpl/test/models/accountDelete.test.ts(5 hunks)packages/xrpl/test/models/accountSet.test.ts(2 hunks)packages/xrpl/test/models/baseTransaction.test.ts(8 hunks)packages/xrpl/test/models/checkCancel.test.ts(3 hunks)packages/xrpl/test/models/checkCash.test.ts(5 hunks)packages/xrpl/test/models/checkCreate.test.ts(7 hunks)packages/xrpl/test/models/clawback.test.ts(8 hunks)packages/xrpl/test/models/depositPreauth.test.ts(9 hunks)packages/xrpl/test/models/escrowCancel.test.ts(2 hunks)packages/xrpl/test/models/escrowCreate.test.ts(3 hunks)packages/xrpl/test/models/escrowFinish.test.ts(5 hunks)packages/xrpl/test/models/offerCancel.test.ts(2 hunks)packages/xrpl/test/models/offerCreate.test.ts(3 hunks)packages/xrpl/test/models/oracleDelete.test.ts(2 hunks)packages/xrpl/test/models/oracleSet.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(5 hunks)packages/xrpl/test/models/paymentChannelClaim.test.ts(3 hunks)packages/xrpl/test/models/paymentChannelCreate.test.ts(2 hunks)packages/xrpl/test/models/paymentChannelFund.test.ts(2 hunks)packages/xrpl/test/models/permissionedDomainDelete.test.ts(2 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(2 hunks)packages/xrpl/test/models/setRegularKey.test.ts(2 hunks)packages/xrpl/test/models/signerListSet.test.ts(3 hunks)packages/xrpl/test/models/ticketCreate.test.ts(1 hunks)packages/xrpl/test/models/trustSet.test.ts(2 hunks)packages/xrpl/test/testUtils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/xrpl/test/models/checkCreate.test.ts
- packages/xrpl/test/models/setRegularKey.test.ts
- packages/xrpl/test/models/escrowFinish.test.ts
- packages/xrpl/test/models/accountSet.test.ts
- packages/xrpl/test/models/signerListSet.test.ts
- packages/xrpl/test/models/checkCash.test.ts
- packages/xrpl/test/models/CredentialDelete.test.ts
- packages/xrpl/test/models/checkCancel.test.ts
- packages/xrpl/test/models/NFTokenAcceptOffer.test.ts
- packages/xrpl/test/models/CredentialAccept.test.ts
- packages/xrpl/test/models/depositPreauth.test.ts
- packages/xrpl/test/models/NFTokenCreateOffer.test.ts
- packages/xrpl/test/models/accountDelete.test.ts
- packages/xrpl/test/models/CredentialCreate.test.ts
- packages/xrpl/test/models/offerCreate.test.ts
🧰 Additional context used
🧠 Learnings (9)
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/clawback.test.ts (1)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
packages/xrpl/test/models/XChainCommit.test.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.
packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
packages/xrpl/test/models/permissionedDomainDelete.test.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
packages/xrpl/test/models/AMMClawback.test.ts (1)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
🪛 GitHub Actions: Node.js CI
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/NFTokenBurn.test.ts
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/clawback.test.ts
[warning] 4-4: 'assertValid' is assigned a value but never used. Allowed unused vars must match /^_/u
[error] 25-25: Functions that return promises must be async
[error] 25-25: Unsafe return of an any typed value
[error] 81-81: Functions that return promises must be async
[error] 81-81: Unsafe return of an any typed value
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts
[error] 19-19: Cannot find name 'assert'.
[error] 19-19: Cannot find name 'validate'.
[error] 29-29: Cannot find name 'assert'.
[error] 29-29: Cannot find name 'validate'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 65-65: Cannot find name 'assertInvalid'.
[error] 69-69: Cannot find name 'assertInvalid'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 19-19: Cannot find name 'assert'.
[error] 19-19: Cannot find name 'validate'.
[error] 29-29: Cannot find name 'assert'.
[error] 29-29: Cannot find name 'validate'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 65-65: Cannot find name 'assertInvalid'.
[error] 69-69: Cannot find name 'assertInvalid'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 19-19: Cannot find name 'assert'.
[error] 19-19: Cannot find name 'validate'.
[error] 29-29: Cannot find name 'assert'.
[error] 29-29: Cannot find name 'validate'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 65-65: Cannot find name 'assertInvalid'.
[error] 69-69: Cannot find name 'assertInvalid'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
packages/xrpl/test/models/NFTokenMint.test.ts
[error] 4-4: Unable to resolve path to module '../../src/models/transactions/nftokenMint'
[error] 4-4: Missing file extension for '../../src/models/transactions/nftokenMint'
[error] 4-4: '../../src/models/transactions/nftokenMint' is not found
packages/xrpl/test/models/NFTokenCancelOffer.test.ts
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/MPTokenAuthorize.test.ts
[error] 2-2: Cannot find module '../../src/models/transactions/mptokenAuthorize' or its corresponding type declarations.
[error] 2-2: Cannot find module '../../src/models/transactions/mptokenAuthorize' or its corresponding type declarations.
[error] 2-2: Cannot find module '../../src/models/transactions/mptokenAuthorize' or its corresponding type declarations.
packages/xrpl/test/models/NFTokenModify.test.ts
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/baseTransaction.test.ts
[error] 4-4: Cannot find module '../../src/models/transactions/BaseTransaction' or its corresponding type declarations.
🪛 Biome (1.9.4)
packages/xrpl/test/models/AMMWithdraw.test.ts
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 101-101: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainClaim.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateBridge.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/DIDSet.test.ts
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 54-54: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 61-61: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 87-87: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 80-80: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 65-65: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 83-83: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 137-137: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 149-149: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 167-167: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCommit.test.ts
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainDelete.test.ts
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 84-84: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 85-85: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateClaimID.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMClawback.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 71-71: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 143-143: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 215-215: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 233-233: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/payment.test.ts
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 83-83: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDelete.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCancel.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/offerCancel.test.ts
[error] 47-47: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleSet.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 128-128: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 142-142: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 149-149: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelClaim.test.ts
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelCreate.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 63-63: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelFund.test.ts
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/trustSet.test.ts
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 ESLint
packages/xrpl/test/models/NFTokenMint.test.ts
[error] 4-4: Missing file extension for "../../src/models/transactions/nftokenMint"
(import/extensions)
packages/xrpl/test/models/MPTokenAuthorize.test.ts
[error] 2-2: Missing file extension for "../../src/models/transactions/mptokenAuthorize"
(import/extensions)
packages/xrpl/test/models/baseTransaction.test.ts
[error] 4-4: Missing file extension for "../../src/models/transactions/BaseTransaction"
(import/extensions)
🪛 Gitleaks (8.21.2)
packages/xrpl/test/models/MPTokenAuthorize.test.ts
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (70)
packages/xrpl/test/models/AMMWithdraw.test.ts (2)
3-9: LGTM! Great refactoring of test utilities.The introduction of
assertTxIsValidandassertTxValidationErrorhelper functions improves code maintainability and readability by centralizing validation logic.
22-86: LGTM! Clean refactoring of validation assertions.The changes consistently apply the new validation pattern across all test cases while maintaining comprehensive test coverage.
packages/xrpl/test/models/NFTokenCancelOffer.test.ts (1)
35-46: LGTM! Test case properly validates empty NFTokenOffers field.The test case correctly validates the empty NFTokenOffers field scenario and follows the standardized error message format.
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/AMMVote.test.ts (3)
4-6: LGTM! Helper functions improve code maintainability.The introduction of
assertValidandassertInvalidfunctions effectively reduces code duplication and improves readability.
16-30: LGTM! Well-structured test setup.The
beforeEachblock provides a comprehensive setup with all required fields and valid values for testing AMMVote transactions.
32-82: LGTM! Comprehensive test coverage.The test suite thoroughly covers all validation scenarios including valid transactions, missing fields, invalid types, and boundary conditions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 61-61: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateClaimID.test.ts (4)
2-7: LGTM! Well-structured helper functions.The introduction of
assertValidandassertInvalidhelper functions improves code maintainability by encapsulating common validation logic. This aligns well with the PR's objective of enhancing validation functions.
14-37: LGTM! Comprehensive test setup.The test setup provides a complete valid transaction object with all required fields, and the explicit
anytype annotation improves type clarity.
39-78: LGTM! Comprehensive test coverage.The test cases thoroughly validate both successful and error scenarios with consistent error message formatting. This aligns well with the PR's objective of enhancing validation functions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
44-44: Regarding thedeleteoperator usage in tests.While static analysis flags the use of
deleteoperator for performance reasons, in this test context:
- Performance impact is negligible as this is test code.
- Using
deleteclearly communicates the intent of testing missing fields.- Using
undefinedassignment might not trigger the same validation behavior.Therefore, I recommend keeping the current implementation.
Also applies to: 56-56, 68-68
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts (4)
2-6: LGTM! Good use of utility functions.The introduction of
assertTxIsValidandassertTxValidationErroralong with helper functions improves code reusability and readability. The type annotations are properly used.
14-14: LGTM! Good type annotation.Adding the type annotation for the test case variable improves code clarity.
25-25: LGTM! Good use of the utility function.Using
assertValidsimplifies the test case and makes it more readable.
31-31: LGTM! Good test case improvements.The test cases are more readable with the
assertInvalidutility function, and using 'abcd' as an invalid value makes the test more explicit.Also applies to: 35-37
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts (1)
40-96: LGTM! Comprehensive test coverage.The test cases thoroughly validate:
- Valid transaction structure
- Required fields (XChainBridge, SignatureReward, Destination, Amount)
- Field type validations
- Consistent error message format
The refactored assertions using
assertValidandassertInvalidimprove code readability.🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 87-87: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts (2)
40-75: LGTM! Well-structured positive test cases.The refactoring improves readability while maintaining comprehensive test coverage for valid transaction scenarios.
77-146: LGTM! Well-structured negative test cases with standardized error messages.The refactoring improves readability and maintains consistent error message format across all validation failures.
🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts (2)
2-6: LGTM! Well-structured utility functions.The introduction of
assertValidandassertInvalidutility functions improves code reusability and readability while following the DRY principle.
65-69: Add missing validation test for FinishAfter.The test case only checks
validateEscrowCreate()but notvalidate(). For consistency with other test cases, add the missing validation test.Apply this diff to add the missing test:
it(`invalid FinishAfter`, function () { escrow.FinishAfter = 'abcd' assertInvalid(escrow, 'EscrowCreate: invalid field FinishAfter') + assert.throws( + () => validate(escrow), + ValidationError, + 'EscrowCreate: invalid field FinishAfter', + ) })packages/xrpl/test/models/paymentChannelClaim.test.ts (1)
45-79: LGTM! The test cases are comprehensive and well-structured.Note: The static analysis warning about using the
deleteoperator can be safely ignored in test code. While usingundefinedassignment would be more performant, thedeleteoperator better expresses the intent of removing fields for testing purposes, and the performance impact is negligible in test scenarios.🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCancel.test.ts (6)
2-6: LGTM! Good use of utility functions.The introduction of
assertValidandassertInvalidhelper functions improves code reusability and makes the test cases more maintainable.
14-14: LGTM! Appropriate use ofletdeclaration.The change from
consttoletis correct as the variable is modified in test cases.
26-26: LGTM! Good use of the new helper function.The change to use
assertValidmakes the test case more concise and readable.
32-32: LGTM! Consistent use of helper functions.The change maintains consistency with other test cases.
38-38: LGTM! Consistent error message format.The error messages now follow a consistent format: "missing/invalid field {fieldName}".
Also applies to: 44-44, 50-50, 56-56
36-36: Note aboutdeleteoperator usage.Static analysis flagged the use of
deleteoperator for potential performance impact. However, in test files:
- Performance is not critical
deleteprovides clearer intent than setting to undefined- Tests often need to simulate missing properties
Therefore, the current implementation is acceptable.
Also applies to: 42-42
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/offerCancel.test.ts (5)
2-6: LGTM! Good refactoring of test utilities.The introduction of
assertValidandassertInvalidutility functions improves code readability and follows the DRY principle by encapsulating common validation logic.
14-14: LGTM! Type annotation is appropriate for test context.Using
anytype provides the necessary flexibility for testing various scenarios.
29-29: No issues with this minor formatting change.
33-33: LGTM! Test cases are now more concise.The use of
assertValidutility function improves test readability while maintaining the same validation logic.Also applies to: 38-38
42-43: LGTM! Test case and error message improvements.The changes improve test clarity by:
- Using a more obvious invalid value ('abcd' instead of a number)
- Following the new consistent error message format
packages/xrpl/test/models/paymentChannelCreate.test.ts (2)
32-102: Well-structured test cases with consistent error messages!The test cases are comprehensive and follow a consistent pattern for error messages. They cover:
- Required fields (Amount, Destination, SettleDelay, PublicKey)
- Optional fields (CancelAfter, DestinationTag, SourceTag)
- Both valid and invalid scenarios
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 63-63: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
37-39: Regarding thedeleteoperator usage in tests.The static analysis tool flags the use of the
deleteoperator for potential performance impacts. However, since this is test code:
- The performance impact is negligible
- Using
deletemakes the intent clearer than setting fields toundefined- It actually removes the property, which is what we want to test
Therefore, we can safely ignore these warnings.
Also applies to: 45-45, 51-51, 57-57, 63-63
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/DIDSet.test.ts (2)
14-31: LGTM! Clean test setup and valid case verification.The test setup provides a comprehensive valid transaction object, and the assertion is clear and concise.
33-49: LGTM! Comprehensive invalid case testing.The test cases effectively verify validation for each field with consistent error message formatting.
packages/xrpl/test/models/trustSet.test.ts (5)
2-6: LGTM! Great refactoring of test assertions.The introduction of
assertTxIsValidandassertTxValidationErrorhelper functions improves code reusability and maintainability across test files.
36-36: LGTM! Clean test assertion.The use of
assertValidhelper function makes the test more readable and consistent with other test files.
46-46: LGTM! Consistent error message format.The error message follows the new standardized format: "invalid field ...".
50-51: LGTM! Clear test case for non-numeric QualityIn.The test value 'abcd' clearly demonstrates the validation of non-numeric input, and the error message follows the standardized format.
55-56: LGTM! Clear test case for non-numeric QualityOut.The test value 'dcba' clearly demonstrates the validation of non-numeric input, and the error message follows the standardized format.
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
48-51: LGTM! Good use of the new assertInvalid helper.The changes align with the PR objective by utilizing the newer
assertInvalidhelper function for validation testing. The error messages are consistent and descriptive.Also applies to: 65-65, 69-69
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 48-48: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/NFTokenMint.test.ts (3)
7-9: LGTM! Well-designed helper functions.The new helper functions
assertValidandassertInvalidimprove code maintainability by:
- Reducing code duplication
- Providing consistent validation behavior
- Making test cases more readable
29-29: LGTM! Consistent URI handling.The URI field is consistently converted to hex format using
stringToHexacross all test cases, improving code consistency.Also applies to: 44-44, 60-60
32-32: LGTM! Clear and consistent error handling.The test cases effectively use the new helper functions with clear, descriptive error messages that follow a consistent format:
- Valid case verification
- Missing field validation
- Account/Issuer equality check
- URI format validation
Also applies to: 47-47, 63-63, 79-79, 95-95
packages/xrpl/test/models/NFTokenModify.test.ts (2)
19-19: LGTM! Consistent use of string conversion utility.The change from
convertStringToHextostringToHexaligns with the standardization effort across the codebase.
33-33: LGTM! Improved error handling consistency.The changes to use
assertInvalidwith descriptive error messages align with the standardization of error handling across transaction tests.Also applies to: 46-46, 59-59
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 33-33: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
5-8: LGTM! Well-structured helper functions.The helper functions
assertValidandassertInvalidare well-designed:
- Improve code reusability and readability
- Follow TypeScript best practices with type annotations
- Encapsulate validation logic effectively
10-16: LGTM! Clear test setup and documentation.The test setup is well-structured with:
- Clear constant naming
- Descriptive JSDoc comments
- Appropriate scope for the test suite
18-63: LGTM! Comprehensive test coverage.The test cases are well-structured and provide good coverage:
- Test valid transaction variations
- Verify required field validation
- Use realistic test data
- Follow the new assertion pattern consistently
This aligns with the learnings that indicate we don't need to test certain negative cases (e.g., invalid address formats) as they're covered elsewhere.
packages/xrpl/test/models/baseTransaction.test.ts (6)
89-89: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the Sequence field, which is a better test for type validation.
117-117: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the LastLedgerSequence field, which is a better test for type validation.
159-159: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the TicketSequence field, which is a better test for type validation.
193-193: LGTM! Error messages standardized.The error messages have been updated to use the standardized format "invalid field Signers", which improves clarity and consistency.
Also applies to: 211-211
232-232: LGTM! Error message standardized.The error message has been updated to use the standardized format "invalid field Memos", which improves clarity and consistency.
240-240: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the NetworkID field, which is a better test for type validation.
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
15-24: LGTM! Type casting approach aligns with best practices.The use of
as anyfor type casting aligns with the preferred approach for validation tests, as it better represents real-world scenarios where the validate method needs to handle arbitrary JSON input from users.packages/xrpl/test/models/AMMDelete.test.ts (1)
44-45: LGTM! Consistent error message format.The error messages follow a consistent pattern using "invalid field" format, which aligns with the PR's refactoring goals.
Also applies to: 56-57
packages/xrpl/test/models/ticketCreate.test.ts (1)
40-43: LGTM! Clear validation boundaries in error messages.The error messages clearly specify the valid range for TicketCount (1 to 250), which helps developers understand the constraints.
packages/xrpl/test/models/paymentChannelFund.test.ts (1)
41-42: LGTM! Consistent error handling pattern.The error messages follow a consistent pattern using "missing field" or "invalid field" format, which aligns with the PR's refactoring goals.
Also applies to: 47-48, 53-54, 59-60, 65-66
packages/xrpl/test/models/XChainModifyBridge.test.ts (1)
46-47: LGTM! Consistent error handling pattern.The error messages follow a consistent pattern using "missing field" or "invalid field" format, which aligns with the PR's refactoring goals.
Also applies to: 52-53, 58-59, 64-67
packages/xrpl/test/models/XChainCreateBridge.test.ts (1)
4-7: LGTM! Helper functions improve test readability.The introduction of
assertValidandassertInvalidhelper functions enhances code organization by encapsulating validation logic.packages/xrpl/test/models/AMMCreate.test.ts (1)
71-81: LGTM! Comprehensive boundary testing for TradingFee.Good test coverage for TradingFee validation, including both upper and lower bounds.
packages/xrpl/test/models/XChainClaim.test.ts (1)
79-83: LGTM! Good validation for DestinationTag type.The test case properly validates that DestinationTag must be a number.
packages/xrpl/test/models/permissionedDomainSet.test.ts (2)
7-10: LGTM! Test utilities improve code maintainability.The introduction of
assertValidandassertInvalidutility functions enhances test readability and reduces code duplication.
18-18: Type declaration aligns with testing best practices.Using
anytype for test input is appropriate here as it better represents real-world scenarios where validation needs to handle arbitrary JSON input.packages/xrpl/test/models/AMMBid.test.ts (1)
37-58: LGTM! Comprehensive validation testing for AuthAccounts.The test cases thoroughly cover various scenarios for AuthAccounts validation, including:
- Array length limits
- Invalid account types
- Null/undefined handling
- Sender address validation
packages/xrpl/test/models/oracleSet.test.ts (1)
6-8: LGTM! Well-structured test refactoring.The helper functions and explicit typing enhance code maintainability while preserving test coverage.
Also applies to: 16-16
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts (1)
4-11: LGTM! Consistent with codebase standards.The helper functions and explicit typing align with the established patterns across the test suite.
Also applies to: 19-19
packages/xrpl/test/models/payment.test.ts (1)
7-9: LGTM! Improved naming and consistent structure.The renaming from
paymentTransactiontopaymentenhances readability, while the helper functions and explicit typing maintain consistency with other test files.Also applies to: 17-17
packages/xrpl/test/testUtils.ts (1)
8-13: LGTM!The imports are well-organized and include the necessary dependencies for the new validation assertion functions.
| it('throws on invalid DIDDelete', function () { | ||
| tx.FakeField = 'blah' | ||
| assert.doesNotThrow(() => validateDIDDelete(tx)) | ||
| assert.doesNotThrow(() => validate(tx)) | ||
| assertValid(tx) | ||
| }) |
There was a problem hiding this comment.
Fix incorrect assertion in invalid field test.
The test case is checking for invalid fields but using assertValid instead of assertInvalid. This will not properly verify that invalid fields are rejected.
Define and use assertInvalid function to properly test invalid fields:
+const assertInvalid = (tx: any, message: string): void =>
+ assertTxValidationError(tx, validateDIDDelete, message)
it('throws on invalid DIDDelete', function () {
tx.FakeField = 'blah'
- assertValid(tx)
+ assertInvalid(tx, 'DIDDelete: invalid field FakeField')
})
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('throws on invalid DIDDelete', function () { | |
| tx.FakeField = 'blah' | |
| assert.doesNotThrow(() => validateDIDDelete(tx)) | |
| assert.doesNotThrow(() => validate(tx)) | |
| assertValid(tx) | |
| }) | |
| const assertInvalid = (tx: any, message: string): void => | |
| assertTxValidationError(tx, validateDIDDelete, message) | |
| it('throws on invalid DIDDelete', function () { | |
| tx.FakeField = 'blah' | |
| assertInvalid(tx, 'DIDDelete: invalid field FakeField') | |
| }) |
There was a problem hiding this comment.
Is there a way in JS to ensure that an object doesn't have any other keys?
There was a problem hiding this comment.
@mvadari Yes, there are a few ways to ensure an object only has specific keys in JavaScript:
- Compare keys with expected set:
function validateKeys(obj, expectedKeys) {
const actualKeys = Object.keys(obj)
const hasOnlyExpectedKeys = actualKeys.every(key => expectedKeys.includes(key))
const hasAllExpectedKeys = expectedKeys.every(key => actualKeys.includes(key))
return hasOnlyExpectedKeys && hasAllExpectedKeys
}- Using TypeScript with strict object literal checks:
interface ValidTransaction {
Account: string;
Fee: string;
Flags: number;
Sequence: number;
TransactionType: string;
}
// TypeScript will error if extra properties are present
const tx: ValidTransaction = {
Account: "...",
Fee: "10",
Flags: 2147483648,
Sequence: 4,
TransactionType: "DIDDelete",
extraField: "invalid" // Error: Object literal may only specify known properties
}- Create a sanitized object with only allowed keys:
function sanitizeObject(obj, allowedKeys) {
return Object.fromEntries(
Object.entries(obj).filter(([key]) => allowedKeys.includes(key))
)
}For your test cases, I'd recommend using TypeScript interfaces with strict checks as it provides compile-time validation, or implementing a validation function that explicitly checks for unexpected keys if you need runtime validation.
✏️ Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.497Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/xrpl/src/models/transactions/batch.ts (1)
140-142: Update error message for consistency.The error message at line 141 uses the old format, while line 123 was updated to be more descriptive. For consistency, this should follow the same pattern.
Apply this diff to align the error message:
if (!isRecord(signerObj)) { - throw new ValidationError(`Batch: BatchSigners[${index}] is not object.`) + throw new ValidationError( + `Batch: invalid field BatchSigners[${index}], expected a valid Record`, + ) }packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
118-140: Add a test: TransferFee present as 0 still requires tfMPTCanTransfer.Spec says the field must not be present without the flag; 0 currently passes due to truthy check.
Proposed test:
+ it('throws w/ TransferFee=0 without tfMPTCanTransfer', function () { + const invalid = { + TransactionType: 'MPTokenIssuanceCreate', + Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm', + TransferFee: 0, + } as any + assertInvalid( + invalid, + 'MPTokenIssuanceCreate: TransferFee cannot be provided without enabling tfMPTCanTransfer flag', + ) + })packages/xrpl/src/models/transactions/vaultCreate.ts (2)
123-136: Empty MPTokenMetadata allowed; violates the error text and differs from IssuanceCreate.If isHex('') returns true, empty metadata passes. Enforce non-empty to match message and IssuanceCreate behavior.
if (tx.MPTokenMetadata !== undefined) { const metaHex = tx.MPTokenMetadata - if (!isHex(metaHex)) { + if (!isHex(metaHex) || metaHex.length === 0) { throw new ValidationError( - 'VaultCreate: MPTokenMetadata must be a valid non-empty hex string', + 'VaultCreate: MPTokenMetadata must be a valid non-empty hex string', ) }
103-112: Now I need to search the codebase directly to verify the validators and the learnings:Based on my verification using the script execution and the retrieved learnings, I can now generate the rewritten review comment.
Inconsistent and under-permissive DomainID validation across transaction types.
validateVaultCreateusesisHexString(validates hex only, omits length check), whilepaymentandofferCreateuseisDomainID(validates both hex and length). Per rippled behavior and confirmed learnings, Domain ID validation should only enforce length (64 characters) at the JS validation layer; hex validation occurs at the serialization layer through Hash types in ripple-binary-codec.Current state: three transactions under-validate, two over-validate.
Update all DomainID call sites to use consistent validation. Consider creating a length-only validator (e.g.,
validateDomainIDLength) or refiningisDomainIDto match rippled enforcement. Priority: alignvalidateVaultCreate,validateVaultSet, andvalidatePermissionedDomainSetwithpaymentandofferCreate—at minimum, add length validation.packages/xrpl/test/models/vaultCreate.test.ts (1)
101-105: Update DomainID test samples to required 64-character length.The test uses 16-character DomainID values ('ABCDEF1234567890'), but the standard requires 64 characters—enforced by the
isDomainIDvalidator used in Payment and OfferCreate transactions. Both test instances at lines 102 and 108 need updating.it('allows DomainID when tfVaultPrivate flag set', function () { - tx.DomainID = 'ABCDEF1234567890' + tx.DomainID = 'ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890' tx.Flags = VaultCreateFlags.tfVaultPrivate assertValid(tx) }) it('throws w/ DomainID set but tfVaultPrivate flag missing', function () { - tx.DomainID = 'ABCDEF1234567890' + tx.DomainID = 'ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890' tx.Flags = 0 assertInvalid( tx, 'VaultCreate: Cannot set DomainID unless tfVaultPrivate flag is set.', ) })packages/xrpl/src/models/transactions/common.ts (1)
780-823: Inconsistent terminology in credential validation error messagesThe
validateCredentialsListfunction handles bothCredentialIDs(strings) andCredentials(AuthorizeCredential objects) based on theisStringIDparameter. However, the error messages are inconsistent:
- Line 792 correctly uses "CredentialIDs" in the field-specific error
- Lines 797, 801, 808, 813, 820 use the generic term "Credentials"
Since line 792 distinguishes by using "CredentialIDs" when appropriate, the other error messages should also be parameterized to use "CredentialIDs" when
isStringIDis true, or consistently use the generic term throughout. This inconsistency propagates to test files (e.g., payment.test.ts lines 258, 265, 275, 286).Consider parameterizing error messages:
+ const fieldName = isStringID ? 'CredentialIDs' : 'Credentials' if (!isArray(credentials)) { throw new ValidationError( - `${transactionType}: invalid field CredentialIDs, expected a valid array`, + `${transactionType}: invalid field ${fieldName}, expected a valid array`, ) } if (credentials.length > maxCredentials) { throw new ValidationError( - `${transactionType}: Credentials length cannot exceed ${maxCredentials} elements`, + `${transactionType}: ${fieldName} length cannot exceed ${maxCredentials} elements`, ) } else if (credentials.length === 0) { throw new ValidationError( - `${transactionType}: Credentials cannot be an empty array`, + `${transactionType}: ${fieldName} cannot be an empty array`, ) } // ... continue for other error messages
♻️ Duplicate comments (1)
packages/xrpl/test/models/payment.test.ts (1)
258-286: Inconsistent terminology: "Credentials" vs "CredentialIDs"Lines 258, 265, 275, and 286 use "Credentials" in error messages, but line 240 correctly uses "CredentialIDs". Since the actual field name is
CredentialIDs(as shown on line 236), all error messages should consistently reference "CredentialIDs" for clarity and to help developers identify the exact field causing validation failures.Apply these changes for consistency:
- const errorMessage = 'Payment: Credentials length cannot exceed 8 elements' + const errorMessage = 'Payment: CredentialIDs length cannot exceed 8 elements'- const errorMessage = 'Payment: Credentials cannot be an empty array' + const errorMessage = 'Payment: CredentialIDs cannot be an empty array'- const errorMessage = 'Payment: Invalid Credentials ID list format' + const errorMessage = 'Payment: Invalid CredentialIDs list format'- const errorMessage = - 'Payment: Credentials cannot contain duplicate elements' + const errorMessage = + 'Payment: CredentialIDs cannot contain duplicate elements'Based on past review comments.
🧹 Nitpick comments (5)
packages/xrpl/test/testUtils.ts (1)
83-92: Static analysis warning is a false positive; regex construction is safe.The ReDoS warning from ast-grep can be safely dismissed because:
- All regex metacharacters are properly escaped using
[.*+?^${}()|[\]\\]before constructing the RegExp- The resulting pattern
^<escaped_string>$with anchors and escaped content cannot cause catastrophic backtracking- This is test utility code with controlled inputs
The regex construction and usage logic is correct and secure.
Optional: Minor documentation nit – Line 86's comment says "TS complains if it's included" but
require-unicode-regexpis an ESLint rule, not a TypeScript compiler constraint. Consider clarifying this refers to ESLint configuration.packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (2)
146-151: Fold empty-hex validation into validateOptionalField to avoid branching on isHex('').Single-source the error and drop the extra emptiness block.
- validateOptionalField(tx, 'MPTokenMetadata', isHexString) + validateOptionalField( + tx, + 'MPTokenMetadata', + (v): v is string => isHexString(v) && v.length > 0, + { invalidMessage: 'expected a valid non-empty hex string' }, + ) - - if (isHexString(tx.MPTokenMetadata) && tx.MPTokenMetadata === '') { - throw new ValidationError( - 'MPTokenIssuanceCreate: MPTokenMetadata must not be empty string', - ) - }
152-160: Remove redundant isHex check; keep only the length cap.isHex already enforced above; only enforce byte-length here.
- if ( - typeof tx.MPTokenMetadata === 'string' && - (!isHex(tx.MPTokenMetadata) || - tx.MPTokenMetadata.length / 2 > MAX_MPT_META_BYTE_LENGTH) - ) { + if ( + typeof tx.MPTokenMetadata === 'string' && + tx.MPTokenMetadata.length / 2 > MAX_MPT_META_BYTE_LENGTH + ) { throw new ValidationError( `MPTokenIssuanceCreate: MPTokenMetadata (hex format) must be non-empty and no more than ${MAX_MPT_META_BYTE_LENGTH} bytes.`, ) }packages/xrpl/src/models/transactions/payment.ts (1)
211-214: Use consistent txType for DomainID errors.Set txType to 'Payment' to match other messages.
- validateOptionalField(tx, 'DomainID', isDomainID, { - txType: 'PaymentTransaction', + validateOptionalField(tx, 'DomainID', isDomainID, { + txType: 'Payment', paramName: 'DomainID', })packages/xrpl/src/models/transactions/common.ts (1)
442-483: Consider minification-safe error message approachThe
invalidMessagesMaprelies oncheckValidity.name(line 527) to look up error messages. As noted in previous reviews, function names can be mangled by minifiers, which would cause fallback to generic "invalid field" messages without the helpful expected-type suffix.For a minification-safe approach, consider attaching a custom
_msgproperty to validator functions or using a WeakMap to associate validators with messages. However, given this is working correctly in the current build pipeline and you've chosen "Chill" review mode, this can be deferred to a future refactor if minification issues arise.Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/xrpl/src/models/common/index.ts(2 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(5 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts(2 hunks)packages/xrpl/src/models/transactions/accountSet.ts(2 hunks)packages/xrpl/src/models/transactions/batch.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(19 hunks)packages/xrpl/src/models/transactions/delegateSet.ts(2 hunks)packages/xrpl/src/models/transactions/escrowCreate.ts(2 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/src/models/transactions/offerCreate.ts(3 hunks)packages/xrpl/src/models/transactions/payment.ts(3 hunks)packages/xrpl/src/models/transactions/vaultCreate.ts(2 hunks)packages/xrpl/src/models/transactions/vaultDeposit.ts(2 hunks)packages/xrpl/src/models/transactions/vaultWithdraw.ts(2 hunks)packages/xrpl/src/sugar/autofill.ts(2 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(3 hunks)packages/xrpl/test/models/batch.test.ts(2 hunks)packages/xrpl/test/models/offerCreate.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(6 hunks)packages/xrpl/test/models/vaultCreate.test.ts(4 hunks)packages/xrpl/test/testUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/xrpl/src/models/transactions/accountSet.ts
- packages/xrpl/src/models/transactions/escrowCreate.ts
- packages/xrpl/src/models/transactions/offerCreate.ts
- packages/xrpl/test/models/batch.test.ts
- packages/xrpl/src/models/transactions/vaultWithdraw.ts
- packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts
- packages/xrpl/test/models/offerCreate.test.ts
- packages/xrpl/src/models/common/index.ts
- packages/xrpl/src/models/transactions/delegateSet.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
📚 Learning: 2025-07-10T22:04:59.994Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Applied to files:
packages/xrpl/src/models/transactions/vaultCreate.ts
📚 Learning: 2024-12-05T16:48:12.951Z
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Applied to files:
packages/xrpl/src/models/transactions/vaultCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/test/models/payment.test.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
📚 Learning: 2024-12-18T20:54:40.935Z
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
🧬 Code graph analysis (7)
packages/xrpl/src/sugar/autofill.ts (1)
packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/test/testUtils.ts (2)
packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/transactions/transaction.ts (1)
validate(231-519)
packages/xrpl/src/models/transactions/vaultCreate.ts (1)
packages/xrpl/src/models/transactions/common.ts (4)
validateOptionalField(547-573)isHexString(179-181)isXRPLNumber(262-269)isNumber(190-198)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (4)
packages/xrpl/src/models/transactions/common.ts (4)
validateOptionalField(547-573)isHexString(179-181)isNumber(190-198)isString(154-156)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/utils/index.ts (1)
isFlagEnabled(28-31)packages/xrpl/src/models/transactions/index.ts (1)
MPTokenIssuanceCreateFlags(59-59)
packages/xrpl/src/models/transactions/payment.ts (1)
packages/xrpl/src/models/transactions/common.ts (9)
validateRequiredField(500-532)isAmount(417-423)isAccount(404-409)validateOptionalField(547-573)isNumber(190-198)isHexString(179-181)validateCredentialsList(781-823)MAX_AUTHORIZED_CREDENTIALS(25-25)isDomainID(871-877)
packages/xrpl/src/models/transactions/common.ts (2)
packages/xrpl/src/models/utils/index.ts (1)
isHex(61-63)packages/xrpl/src/models/common/index.ts (4)
Currency(21-21)IssuedCurrency(12-15)MPTCurrency(17-19)XRPAmount(23-23)
packages/xrpl/src/models/transactions/vaultDeposit.ts (1)
packages/xrpl/src/models/transactions/common.ts (2)
validateRequiredField(500-532)isHexString(179-181)
🪛 ast-grep (0.39.6)
packages/xrpl/test/testUtils.ts
[warning] 84-88: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
// eslint-disable-next-line require-unicode-regexp -- TS complains if it's included
^${errorMessage.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}$,
'u',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Analyze (javascript)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (17)
packages/xrpl/src/models/transactions/batch.ts (1)
121-125: LGTM! Error message improvement.The updated error message is more specific and descriptive. Using "expected a valid Record" is more precise than "is not object" since
isRecordchecks for plain objects with string keys, not just any object type.packages/xrpl/test/testUtils.ts (1)
78-82: LGTM! Flexible parameter type enhances test capabilities.Adding support for
RegExpin addition tostringfor theerrorMessageparameter is a valuable enhancement that allows test writers to use pattern matching while maintaining backward compatibility.packages/xrpl/src/sugar/autofill.ts (3)
122-125: Good improvement: using presence check for Destination.Switching from a truthy check to the
inoperator is more robust, as it correctly handles cases whereDestinationexists but has a falsy value.
149-158: Good refactor: explicit validation improves clarity.The addition of explicit presence checking (line 149) and type validation (line 154) makes the validation logic clearer and more robust. The introduction of the local
valvariable ensures type safety after validation.
163-163: Good consistency: using presence check for tagField.This change aligns with the presence check pattern used elsewhere in the function, correctly handling cases where the tag field might have a falsy value like 0.
packages/xrpl/src/models/transactions/index.ts (1)
41-43: Export paths verified—naming is consistent and files exist.Verification confirms camelCase paths in lines 41-43 correctly reference actual module files (
credentialAccept.ts,credentialCreate.ts,credentialDelete.ts). No broken imports or PascalCase remnants found. The naming aligns with other non-acronym transaction exports, and CI passing validates module resolution.packages/xrpl/src/models/transactions/vaultCreate.ts (1)
148-160: LGTM on metadata warnings path.Console warning aggregation is clean and mirrors IssuanceCreate.
packages/xrpl/src/models/transactions/payment.ts (1)
194-204: LGTM: Centralized validation and Paths predicate.Good use of validateRequired/Optional and a clear isPaths type guard.
Also applies to: 286-301
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
41-53: Consolidate empty string validation into a single code path to avoid dual error messages.The current implementation uses
validateOptionalFieldwithisHexString(which accepts empty strings sinceHEX_REGEXmatches*zero or more characters), then adds a separate check at line 146-150 to catch empty strings with a distinct error message. This creates two validation paths for one field:
- Empty string
''passesisHexString, caught by line 146 →"must not be empty string"- Non-hex string
'http://xrpl.org'failsisHexString, caught byvalidateOptionalField→"expected a valid hex string"Consider using the already-imported
validateMPTokenMetadata(line 14) as a wrapper validator that handles both hex format and non-emptiness checks, replacing the dual validation logic at lines 142 and 146-150 with a singlevalidateOptionalFieldcall.packages/xrpl/src/models/transactions/vaultDeposit.ts (1)
40-41: Review comment is incorrect. VaultID validation requires no changes.VaultID is consistently validated with
isHexStringacross all five vault transaction types, and test data confirms VaultID length is 16 hex characters (8 bytes), not 64. This differs from DomainID (which is 64 chars and has its ownisDomainIDvalidator in Payment/OfferCreate). The current validation approach is intentional and consistent with the codebase design.Likely an incorrect or invalid review comment.
packages/xrpl/src/models/transactions/common.ts (7)
138-181: LGTM: Clean type guard additionsThe new type guards
isArray,isCurrencyString, andisHexStringare well-implemented and follow TypeScript type guard conventions correctly. TheisCurrencyStringfunction appropriately handles both standard (3-letter) and non-standard (40-character hex) currency codes as per XRPL specifications.
190-217: LGTM: Empty string edge case properly handledThe updated
isNumberfunction now correctly rejects empty strings with thenum.trim() !== ''check on line 194, addressing the concern raised in previous reviews aboutNumber('')evaluating to0. TheisNumberWithBoundsfactory function is well-structured and the returned function name matches the invalidMessagesMap entry.
316-318: LGTM: XRP amount validation is correctThe
isXRPAmountimplementation correctly validates that the input is both a string and a valid numeric value. SinceisNumbernow rejects empty strings (line 194), the concern from previous reviews about empty strings being treated as "0 drops" has been resolved.
500-573: LGTM: Centralized error messaging improves consistencyThe additions of the
invalidMessageparameter to bothvalidateRequiredFieldandvalidateOptionalField, along with the integration ofgetErrorMessage, provide a consistent and maintainable approach to error message generation across all transaction validators.
674-730: LGTM: validateBaseTransaction properly refactoredThe refactored
validateBaseTransactionnow leverages the new validation helpers consistently:
Accountcorrectly validated withisAccount(not just string check)Feevalidated asisXRPAmount- Hex fields (
AccountTxnID,SigningPubKey,TxnSignature) useisHexStringFlagsaccepts both number and object with a clear custom messageMemosandSignershave field-specific error messagesThe empty
SigningPubKeycase for multi-signature is correctly handled sinceisHexStringnow accepts empty strings.
72-94: LGTM: Memo validation strengthened with isHexStringThe
isMemofunction now correctly usesisHexStringfor validatingMemoData,MemoFormat, andMemoTypefields (lines 82-84), ensuring that memo fields are properly hex-encoded as per XRPL specifications. This is an improvement over any previous looser validation.Based on learnings.
431-439: LGTM: XChainBridge validation improved with explicit type guardsThe refactored
isXChainBridgenow uses explicit type guards (isStringfor door addresses andisCurrencyfor currency objects), providing clearer validation and better error messages compared to implicit checks.Based on learnings.
| validateOptionalField(tx, 'MaximumAmount', isString) | ||
| validateOptionalField(tx, 'MPTokenMetadata', isString) | ||
| validateOptionalField(tx, 'MPTokenMetadata', isHexString) | ||
| validateOptionalField(tx, 'TransferFee', isNumber) | ||
| validateOptionalField(tx, 'AssetScale', isNumber) | ||
|
|
There was a problem hiding this comment.
AssetScale should be non-negative.
isNumber accepts negative integers; add a non-negative check per doc.
validateOptionalField(tx, 'AssetScale', isNumber)
+ if (isNumber(tx.AssetScale) && Number(tx.AssetScale) < 0) {
+ throw new ValidationError(
+ 'MPTokenIssuanceCreate: AssetScale must be a non-negative integer',
+ )
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validateOptionalField(tx, 'MaximumAmount', isString) | |
| validateOptionalField(tx, 'MPTokenMetadata', isString) | |
| validateOptionalField(tx, 'MPTokenMetadata', isHexString) | |
| validateOptionalField(tx, 'TransferFee', isNumber) | |
| validateOptionalField(tx, 'AssetScale', isNumber) | |
| validateOptionalField(tx, 'MaximumAmount', isString) | |
| validateOptionalField(tx, 'MPTokenMetadata', isHexString) | |
| validateOptionalField(tx, 'TransferFee', isNumber) | |
| validateOptionalField(tx, 'AssetScale', isNumber) | |
| if (isNumber(tx.AssetScale) && Number(tx.AssetScale) < 0) { | |
| throw new ValidationError( | |
| 'MPTokenIssuanceCreate: AssetScale must be a non-negative integer', | |
| ) | |
| } |
🤖 Prompt for AI Agents
In packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts around lines
141 to 145, the AssetScale field is currently validated with isNumber which
permits negative values; update the validation so AssetScale, when present, must
be a non-negative number (e.g., replace the validator with a predicate that
checks isNumber(value) && value >= 0 or use an existing nonNegativeNumber
helper) to enforce the documented constraint.
| if (isNumber(tx.TransferFee)) { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Not necessary | ||
| const flags = (tx.Flags ?? 0) as | ||
| | number | ||
| | MPTokenIssuanceCreateFlagsInterface | ||
| const isTfMPTCanTransfer = | ||
| typeof flags === 'number' | ||
| ? isFlagEnabled(flags, MPTokenIssuanceCreateFlags.tfMPTCanTransfer) | ||
| : (flags.tfMPTCanTransfer ?? false) | ||
| const flags = (tx.Flags ?? 0) as number | Record<string, unknown> | ||
| const isTfMPTCanTransfer = isNumber(flags) | ||
| ? isFlagEnabled(flags, MPTokenIssuanceCreateFlags.tfMPTCanTransfer) | ||
| : flags.tfMPTCanTransfer === true | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
TransferFee presence check ignores 0; enforce presence regardless of value.
Field must not be present without tfMPTCanTransfer.
- if (tx.TransferFee && !isTfMPTCanTransfer) {
+ if (tx.TransferFee !== undefined && !isTfMPTCanTransfer) {
throw new ValidationError(
'MPTokenIssuanceCreate: TransferFee cannot be provided without enabling tfMPTCanTransfer flag',
)
}Also applies to: 188-192
🏁 Script executed:
#!/bin/bash
# Find tests missing the TransferFee=0 case
rg -n "TransferFee\\s*:\\s*0" packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts -C2 || trueLength of output: 0
I need to gather more information by inspecting the actual code and searching more broadly for test coverage. Let me verify the implementation and test cases.
🏁 Script executed:
#!/bin/bash
# View the actual code at lines 175-181 and 188-192
echo "=== Lines 175-181 ==="
sed -n '175,181p' packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
echo ""
echo "=== Lines 188-192 ==="
sed -n '188,192p' packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
echo ""
echo "=== Broader search for TransferFee in tests ==="
rg -n "TransferFee" packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts -C2 || echo "No matches found"Length of output: 2223
Bug confirmed: presence check for TransferFee incorrectly ignores 0 value.
The current validation at line 188 uses if (tx.TransferFee && !isTfMPTCanTransfer), which fails to validate when TransferFee is set to 0 (a falsy value). Since 0 is a valid value within the allowed range (0-50000), it should still require the tfMPTCanTransfer flag to be enabled. The proposed fix to use tx.TransferFee !== undefined is correct and necessary.
Test coverage shows no test case for the edge case of TransferFee: 0 without the required flag, which would currently pass validation incorrectly.
🤖 Prompt for AI Agents
In packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts around lines
175 to 181, the present validation uses a truthy check for tx.TransferFee which
incorrectly ignores a valid value of 0; change the conditional to test for
defined-ness (tx.TransferFee !== undefined) so that 0 still triggers the
requirement for the tfMPTCanTransfer flag, update the surrounding logic to use
the computed isTfMPTCanTransfer value accordingly, and add a unit test covering
the edge case TransferFee: 0 without the flag to ensure it fails validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/common.ts (2)
269-277: LGTM!isXRPAmountcorrectly validates XRP drop amounts.The implementation reuses
isNumber(which now rejects empty strings), addressing the past review concern about empty string acceptance. The docstring should be updated though—it says "IssuedCurrencyAmount" but validates XRP amounts.🔎 Fix docstring
/** - * Verify the form and type of an IssuedCurrencyAmount at runtime. + * Verify the form and type of an XRPAmount (drops) at runtime. * * @param input - The input to check the form and type of. - * @returns Whether the IssuedCurrencyAmount is properly formed. + * @returns Whether the XRPAmount is properly formed. */ export function isXRPAmount(input: unknown): input is XRPAmount {
827-831: Minor: Weak type guard inisAuthorizeCredentialArray.The comment acknowledges this is not a rigorous type guard. While acceptable for internal use, consider adding a note that this assumes the array is non-empty and homogeneous, or add a more thorough check if this function is used in contexts where the assumption might not hold.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/index.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/src/models/transactions/vaultCreate.tspackages/xrpl/src/sugar/autofill.tspackages/xrpl/test/models/MPTokenIssuanceCreate.test.tspackages/xrpl/test/models/vaultCreate.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/xrpl/src/sugar/autofill.ts
- packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
- packages/xrpl/src/models/transactions/index.ts
- packages/xrpl/src/models/transactions/vaultCreate.ts
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.784Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
📚 Learning: 2025-06-26T17:25:36.429Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-10-30T12:09:55.784Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.784Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-06T19:25:15.376Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-10-28T14:16:24.585Z
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-08T02:08:00.476Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/src/models/ledger/PermissionedDomain.ts:3-8
Timestamp: 2025-01-08T02:08:00.476Z
Learning: In xrpl.js, the Credential interface must maintain a nested structure with a `Credential` property containing `Issuer` and `CredentialType` fields to mirror the structure defined in the rippled codebase.
Applied to files:
packages/xrpl/src/models/common/index.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-12T01:12:01.868Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).
Applied to files:
packages/xrpl/src/models/transactions/transaction.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-10-02T15:47:02.491Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Applied to files:
packages/xrpl/src/models/transactions/transaction.tspackages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-04-16T15:22:45.633Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Applied to files:
packages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-02-12T23:30:40.622Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.622Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
Applied to files:
packages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-02-12T23:28:55.377Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Applied to files:
packages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
📚 Learning: 2025-10-28T14:16:24.585Z
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
📚 Learning: 2025-09-16T05:00:20.420Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.420Z
Learning: In XRPL's MPTokenIssuanceCreate flags, the tfMPTCanMutate* flags intentionally use the same bit values as their corresponding base flags (e.g., tfMPTCanMutateCanLock = 0x00000002 matches tfMPTCanLock = 0x00000002) because they operate in different contexts - base flags go in the Flags field while mutable flags go in the MutableFlags field. This design avoids ambiguity since the flags are applied to separate bit fields.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
📚 Learning: 2025-07-10T22:04:59.994Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-05T16:48:12.951Z
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-12-18T20:54:40.935Z
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.tspackages/xrpl/src/models/transactions/common.ts
📚 Learning: 2024-10-30T01:05:33.583Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2812
File: packages/xrpl/test/integration/transactions/payment.test.ts:41-43
Timestamp: 2024-10-30T01:05:33.583Z
Learning: In tests involving fee calculations for XRPL, avoid using default fee amounts. If unable to retrieve fee values from the latest validated ledger, throw an error instead of defaulting to a specific amount.
Applied to files:
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts
📚 Learning: 2024-10-08T16:29:11.194Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2795
File: packages/xrpl/src/models/transactions/common.ts:84-84
Timestamp: 2024-10-08T16:29:11.194Z
Learning: When dealing with `bigint` values, avoid converting them to `number` to prevent loss of precision.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-07-11T21:22:07.809Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-11T21:22:07.809Z
Learning: Domain ID validation in XRPL.js is handled at the serialization/deserialization layer through Hash types in the ripple-binary-codec package, not at the transaction validation layer. The validateDomainID function only needs to validate length (64 characters) as hex validation occurs when Hash types are constructed during serialization.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-17T09:27:33.335Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:32-33
Timestamp: 2025-01-17T09:27:33.335Z
Learning: Use `Array.isArray()` instead of `instanceof Array` for reliable array type checking in JavaScript, as `instanceof Array` fails for cross-realm arrays (e.g., from iframes) and arrays from different JavaScript contexts.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
📚 Learning: 2025-01-14T20:47:34.164Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/src/models/transactions/permissionedDomainSet.ts:14-14
Timestamp: 2025-01-14T20:47:34.164Z
Learning: The PermissionedDomain's AcceptedCredentials array has a maximum length of 10 as per XLS-80d specification, which is different from the credentials feature limit. These limits should not be aligned as they serve different purposes.
Applied to files:
packages/xrpl/src/models/transactions/common.ts
🧬 Code graph analysis (1)
packages/xrpl/src/models/transactions/common.ts (2)
packages/xrpl/src/models/utils/index.ts (1)
isHex(61-63)packages/xrpl/src/models/common/index.ts (4)
Currency(21-21)IssuedCurrency(12-15)MPTCurrency(17-19)XRPAmount(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: unit (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (24.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (14)
packages/xrpl/src/models/transactions/transaction.ts (1)
26-28: LGTM! Naming consistency improved.The import path changes from PascalCase to camelCase align these Credential modules with the existing convention used by most other transaction types in this file (e.g.,
accountDelete,accountSet,checkCancel). The public API remains unchanged since the named exports retain their PascalCase identifiers.packages/xrpl/src/models/common/index.ts (1)
23-24: LGTM! Semantic type alias improves clarity.The introduction of
XRPAmountas a type alias forstringand its use in theAmountunion type improves code readability and self-documentation without introducing breaking changes. This aligns well with the broader refactor to provide more explicit type information.Also applies to: 35-35
packages/xrpl/test/models/vaultCreate.test.ts (3)
22-22: LGTM! Usinganytype in tests is appropriate.Based on learnings, using
as anytype assertions is acceptable in test files for the XRPL.js project. This approach better represents real-world scenarios where thevalidatemethod handles arbitrary JSON input from users.
56-70: LGTM! Updated error messages align with centralized validation.The updated assertions correctly reflect the new, more descriptive error message format from the refactored validation helpers. The messages now include field names and expected types (e.g., "expected a valid Currency", "expected a valid hex string"), which improves debuggability.
93-99: LGTM! Consistent error message format for WithdrawalPolicy.The test correctly expects the new standardized error message format for invalid number fields.
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (2)
143-151: LGTM! Enhanced validation for MPTokenMetadata.The change from
isStringtoisHexStringensures the field is validated as a hex string rather than just any string. The explicit empty string check (lines 147-151) correctly rejects empty metadata before the length validation at lines 153-161.
176-181: Widened flags type is reasonable for runtime flexibility.The change from
MPTokenIssuanceCreateFlagsInterfacetoRecord<string, unknown>for the flags type acknowledges that at runtime, the flags object may not conform exactly to the expected interface. The logic correctly handles both numeric flags and object-based flags.packages/xrpl/src/models/transactions/common.ts (7)
97-105: LGTM! NewisArraytype guard is well-implemented.The guard correctly uses
Array.isArray()as per learnings (preferred overinstanceof Arrayfor cross-realm safety) and includes a null check.
117-130: LGTM!isCurrencyStringprovides clear validation for currency codes.The guard correctly validates both standard 3-letter currency codes (e.g., "USD") and non-standard 40-character hex currency codes. This centralizes currency string validation logic.
138-140: LGTM!isHexStringconsolidates type and format check.This guard addresses the earlier concern about
isHexassuming the input is a string. It now checks bothtypeof str === 'string'andisHex(str).
149-157: LGTM!isNumbernow correctly rejects empty strings.The updated implementation addresses the past review concern about
Number('')evaluating to0. The checknum.trim() !== ''prevents empty or whitespace-only strings from being accepted as valid numbers.
168-176: LGTM!isNumberWithBoundsis a useful helper for bounded numeric validation.The higher-order function pattern allows reusable bounded checks (e.g., for TradingFee, TickSize). The returned function has an explicit name (
isNumberWithBoundsInternal) which is mapped ininvalidMessagesMap.
236-267: LGTM! Currency type guards are well-structured.The decomposition into
isXRPCurrency,isIssuedCurrency, andisMPTCurrencywithisCurrencyas their composition provides clear, reusable validation. Each guard correctly validates the expected object shape and field count.
682-723: LGTM!validateBaseTransactionuses new centralized validators.The updated validation correctly uses
isAccount,isXRPAmount,isHexString, and other new guards. The custom inline validator forFlags(line 698) handles both numeric and object forms. Error messages forMemosandSignersare appropriately simplified since these have complex validation logic.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/test/models/vaultCreate.test.ts (1)
193-196:⚠️ Potential issue | 🔴 CriticalBoth the unused
@ts-expect-errordirective and the incorrect error message expectation need to be fixed.Since
txis typed asanyon line 22, the@ts-expect-erroron line 194 is unnecessary and should be removed.More critically, the expected error message is incorrect. When
Scaleis set to'invalid'for an IOU asset, the validation throws'VaultCreate: Scale must be a number between 0 and 18 inclusive for IOU assets', not'VaultCreate: invalid field Scale'. This test will fail with the current expectation.Fix required
- // `@ts-expect-error` for test tx.Scale = 'invalid' - assertInvalid(tx, 'VaultCreate: invalid field Scale') + assertInvalid(tx, 'VaultCreate: Scale must be a number between 0 and 18 inclusive for IOU assets')
3 participants
High Level Overview of Change
This PR cleans up the validation functions for transactions, to use newer helper functions. It also adds some helper functions for easier model validation test writing.
This process uncovered a few bugs, which are fixed.
The integration tests have not been touched at all, which shows that nothing major was changed.
Note for reviewers: the vast majority of the changes were made with regex find-and-replace, so while there are a lot of changes, most of them (especially in the test files) are pretty formulaic.
Context of Change
The
validateRequiredFieldandvalidateOptionalFieldhelper functions were added somewhat recently, and the older models don't use it.Type of Change
Did you update HISTORY.md?
Test Plan
Fixed tests. CI passes.